London | 26-ITP-SEP | Abdennour Hachemi | Sprint 1 | form-controls - #1503
London | 26-ITP-SEP | Abdennour Hachemi | Sprint 1 | form-controls#1503AbdennourHachemi wants to merge 7 commits into
Conversation
❌ Deploy Preview for cyf-onboarding-module failed. Why did it fail? →
|
AbdennourHachemi
added
the
Needs Review
abdishakoor-dev
left a comment
abdishakoor-dev
left a comment
There was a problem hiding this comment.
Good start, and the coloured squares behind the colour options are a nice touch. Things to sort before it can be marked Complete:
- No submit button, so the form can't be submitted and none of the validation ever runs. See line 90.
- Name isn't required. The pattern is there, but without
requiredan empty name goes through. See line 20. - Size isn't required. See line 55.
- A stray full stop on line 50 shows up on the page, and the HTML validator from the README checklist (https://validator.w3.org/) reports one error, on line 39.
- Formatting. Prettier fails on both files. Run
npx prettier --write Form-Controlsfrom the repo root and push.
Add the Needs Review label again once you've pushed.
| </div> | ||
| </div> | ||
| </div> | ||
| </form> |
There was a problem hiding this comment.
The form closes here with no submit button. Open the deploy preview and try to send the form. What's missing, and what does the browser do with required and pattern until it's there?
| <main> | ||
| <form> | ||
| <div id="fn"><label for="fname">Please Enter your First name:</label> | ||
| <input type="text" id="fname" name="fname" pattern=".*\S.*\S.*"> <br> |
There was a problem hiding this comment.
The pattern attribute is right and does what the README asks, but the field isn't required. Once you've added a submit button, leave the name empty and submit. It will go through. Which attribute is missing? Also, the task asks for one name field, not first and last.
| <div class="sizeOption"> | ||
| <label for="xs">XS</label> | ||
| <div> | ||
| <input type="radio" name="size" value="xs" id="xs"> |
There was a problem hiding this comment.
None of the six size radios has required, so a size is optional. You've already solved this for colour on line 45. Same thing here.
| <input type="email" id="email" name="email" required><br> | ||
| </div> | ||
| <div class="colorSelection"> | ||
| <label>Please pick a color:</label> <br> |
There was a problem hiding this comment.
This label isn't attached to any control, it's just acting as a heading for the group. Which element does the MDN page from the prep use to group a set of radio buttons and give them a caption? Same for line 50.
| </div> | ||
| </div> | ||
| <div id="sizediv"> | ||
| <label>Please select the t-shirt size:</label> <br> . |
There was a problem hiding this comment.
There's a stray . after the <br> here and it shows on the page.
| <div class="colorOption"> | ||
| <label for="greenOption">Green</label> | ||
| <div class="square" id="greenOptions"> | ||
| <input type="radio" name="color" id="greenOption"value="green"> |
There was a problem hiding this comment.
Missing space between id="greenOption" and value="green". This is the one error the HTML validator (https://validator.w3.org/, from the README checklist) reports. The formatter will fix it.
abdishakoor-dev
added
Reviewed
2 participants
Self checklist
Task code
CYF-1004
A solution to the T-shirt order form using HTML , i have added an external css file , had to close to first pull request since there were some accessiblity issues, I was confused on how to do the PR , but the instructions and searching the net was helpful.